Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new cronjob primitive to the Operator Component Framework, extending the primitives layer with first-class support for reconciling Kubernetes batch/v1 CronJobs (builder/resource, mutator + editors, flavors), plus accompanying docs and an example.
Changes:
- Introduces
pkg/primitives/cronjobwith builder/resource lifecycle integration, mutation planning/apply, flavors, and default handlers. - Adds new mutation editors for
CronJobSpecandJobSpecunderpkg/mutation/editors. - Adds docs and an end-to-end example; also updates repo lint config and a few existing test files.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/primitives/cronjob/resource.go | CronJob primitive resource wrapper around generic IntegrationResource |
| pkg/primitives/cronjob/mutator.go | Plan/apply mutator for CronJob + containers/initContainers editing |
| pkg/primitives/cronjob/handlers.go | Default operational + suspension handlers for CronJob |
| pkg/primitives/cronjob/flavors.go | Field-application flavors for labels/annotations + pod template metadata |
| pkg/primitives/cronjob/builder.go | Fluent builder wiring defaults, mutations, flavors, handlers, extractors |
| pkg/primitives/cronjob/mutator_test.go | Extensive mutator behavior tests (ordering, selectors, nil-safety, etc.) |
| pkg/primitives/cronjob/handlers_test.go | Tests for default operational/suspension handlers |
| pkg/primitives/cronjob/flavors_test.go | Tests for flavor behavior and ordering |
| pkg/primitives/cronjob/builder_test.go | Builder validation and option wiring tests |
| pkg/mutation/editors/cronjobspec.go | New typed editor for batchv1.CronJobSpec |
| pkg/mutation/editors/cronjobspec_test.go | Tests for CronJobSpecEditor methods |
| pkg/mutation/editors/jobspec.go | New typed editor for batchv1.JobSpec |
| pkg/mutation/editors/jobspec_test.go | Tests for JobSpecEditor methods |
| pkg/component/suite_test.go | Removes revive nolint from dot-imports |
| pkg/component/component_test.go | Removes revive nolint from dot-imports |
| internal/generic/resource_task_test.go | Removes dupl nolint marker |
| internal/generic/resource_integration_test.go | Removes dupl nolint marker |
| examples/cronjob-primitive/resources/cronjob.go | Example resource factory assembling CronJob primitive with features/flavors |
| examples/cronjob-primitive/main.go | Runnable fake-client reconciliation demo exercising feature/suspend flows |
| examples/cronjob-primitive/features/mutations.go | Example feature mutations using the cronjob mutator/editors |
| examples/cronjob-primitive/app/controller.go | Example controller wiring a CronJob-backed component |
| examples/cronjob-primitive/README.md | Documentation for running and understanding the example |
| docs/primitives/cronjob.md | New primitive documentation for CronJob |
| .golangci.yml | Updates golangci-lint v2 config layout, enabled linters, and exclusions |
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
<!-- claude-review-cycle --> |
JobSpecEditor provides typed methods for Job spec fields (completions, parallelism, backoff limit, etc.). CronJobSpecEditor provides typed methods for CronJob spec fields (schedule, concurrency policy, time zone, etc.). The suspend field is intentionally excluded from CronJobSpecEditor as it is managed by the framework's suspension system. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Implements the CronJob primitive using IntegrationBuilder with Operational and Suspendable concepts. Key behaviors: - Operational status based on LastScheduleTime (Pending vs Operational) - Suspension via spec.suspend=true (DeleteOnSuspend=false) - Full pod-template mutation pipeline: metadata, cronjob spec, job spec, pod template metadata, pod spec, containers, init containers - Flavors for preserving labels, annotations, and pod template metadata Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the Integration lifecycle, mutation pipeline ordering, editors (CronJobSpec, JobSpec, PodSpec, Container, ObjectMeta), operational status semantics, and suspension behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Demonstrates building a CronJob resource with version-gated mutations, tracing/metrics features, field preservation flavors, suspension via spec.suspend, and data extraction. Uses the shared ExampleApp CRD and a fake client for standalone execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix container field paths in docs mutation ordering table to use full CronJob path (spec.jobTemplate.spec.template.spec.containers) - Use correct OperationPending status name in docs to match framework constant - Explicitly convert LastScheduleTime to UTC before formatting timestamp - Fix range variable address-of footgun in mutator_test.go findEnv helper Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix capabilities table in cronjob.md to use `OperationPending` instead of `Pending` to match the actual status value from the concepts package. Add resource_test.go for the cronjob primitive covering Identity, Object, Mutate, feature ordering, custom field applicator, ConvergingStatus, DeleteOnSuspend, Suspend, SuspensionStatus, and ExtractData. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
563d078 to
5058194
Compare
|
approved |
The FeatureMutator interface in internal/generic used an unexported beginFeature() method, which meant primitive mutators in external packages (cronjob, deployment, configmap) could never satisfy the interface. The type assertion in ApplyMutations always returned false, so feature boundaries were silently skipped for all primitives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored:
|
| default: none | ||
| enable: |
There was a problem hiding this comment.
This golangci-lint config restructuring is very likely invalid for golangci-lint YAML schema: keys like linters.default, linters.settings, and linters.exclusions (and formatters.exclusions) are not recognized in commonly supported versions, which can cause golangci-lint to error and skip linting entirely. Recommend reverting to the known-good schema (linters: disable-all/enable, linters-settings:, issues: exclude-rules: / issues: exclusions: depending on version) and validating with golangci-lint config verify (or running golangci-lint in CI) before merging.
| - unused | ||
| - whitespace | ||
|
|
||
| settings: |
There was a problem hiding this comment.
This golangci-lint config restructuring is very likely invalid for golangci-lint YAML schema: keys like linters.default, linters.settings, and linters.exclusions (and formatters.exclusions) are not recognized in commonly supported versions, which can cause golangci-lint to error and skip linting entirely. Recommend reverting to the known-good schema (linters: disable-all/enable, linters-settings:, issues: exclude-rules: / issues: exclusions: depending on version) and validating with golangci-lint config verify (or running golangci-lint in CI) before merging.
| - name: unexported-return | ||
| - name: indent-error-flow | ||
| - name: errorf | ||
| exclusions: |
There was a problem hiding this comment.
This golangci-lint config restructuring is very likely invalid for golangci-lint YAML schema: keys like linters.default, linters.settings, and linters.exclusions (and formatters.exclusions) are not recognized in commonly supported versions, which can cause golangci-lint to error and skip linting entirely. Recommend reverting to the known-good schema (linters: disable-all/enable, linters-settings:, issues: exclude-rules: / issues: exclusions: depending on version) and validating with golangci-lint config verify (or running golangci-lint in CI) before merging.
| issues: | ||
| max-issues-per-linter: 0 | ||
| max-same-issues: 0 | ||
| formatters: |
There was a problem hiding this comment.
This golangci-lint config restructuring is very likely invalid for golangci-lint YAML schema: keys like linters.default, linters.settings, and linters.exclusions (and formatters.exclusions) are not recognized in commonly supported versions, which can cause golangci-lint to error and skip linting entirely. Recommend reverting to the known-good schema (linters: disable-all/enable, linters-settings:, issues: exclude-rules: / issues: exclusions: depending on version) and validating with golangci-lint config verify (or running golangci-lint in CI) before merging.
| - name: unexported-return | ||
| - name: indent-error-flow | ||
| - name: errorf | ||
| exclusions: |
There was a problem hiding this comment.
This golangci-lint config restructuring is very likely invalid for golangci-lint YAML schema: keys like linters.default, linters.settings, and linters.exclusions (and formatters.exclusions) are not recognized in commonly supported versions, which can cause golangci-lint to error and skip linting entirely. Recommend reverting to the known-good schema (linters: disable-all/enable, linters-settings:, issues: exclude-rules: / issues: exclusions: depending on version) and validating with golangci-lint config verify (or running golangci-lint in CI) before merging.
|
|
||
| | Capability | Detail | | ||
| | ------------------------ | ------------------------------------------------------------------------------------------- | | ||
| | **Operational tracking** | Reports `OperationPending` (never scheduled) or `Operational` (has scheduled at least once) | |
There was a problem hiding this comment.
The docs reference OperationPending, but the framework concepts in code use OperationalStatusPending (and the tests assert concepts.OperationalStatusPending). Please align the documentation to the actual status names returned by the primitive to avoid confusing API consumers.
| | **Operational tracking** | Reports `OperationPending` (never scheduled) or `Operational` (has scheduled at least once) | | |
| | **Operational tracking** | Reports `OperationalStatusPending` (never scheduled) or `Operational` (has scheduled at least once) | |
| | Status | Condition | | ||
| | ------------------ | -------------------------------- | | ||
| | `OperationPending` | `Status.LastScheduleTime == nil` | | ||
| | `Operational` | `Status.LastScheduleTime != nil` | |
There was a problem hiding this comment.
The docs reference OperationPending, but the framework concepts in code use OperationalStatusPending (and the tests assert concepts.OperationalStatusPending). Please align the documentation to the actual status names returned by the primitive to avoid confusing API consumers.
Claude Review Cycle 1 CompleteAddressed: Intentionally ignored:
<!-- claude-review-cycle --> |
| func (m *Mutator) EditObjectMetadata(edit func(*editors.ObjectMetaEditor) error) { | ||
| if edit == nil { | ||
| return | ||
| } | ||
| m.active.cronjobMetadataEdits = append(m.active.cronjobMetadataEdits, edit) | ||
| } |
There was a problem hiding this comment.
All the mutator registration methods append to m.active without guarding against m.active being nil. If any caller forgets to call BeginFeature() (or calls helper methods before BeginFeature), this will panic at runtime. Consider making the mutator safe by either (a) initializing a default plan in NewMutator, (b) lazily calling BeginFeature() the first time a registration method is called when m.active is nil, or (c) returning/recording an explicit error state (or at least a clear panic message) when BeginFeature hasn't been called.
…ations Add requireActive() guard to all mutator registration methods so that callers get an explicit panic message instead of a nil pointer dereference if they forget to call BeginFeature() first. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Review Cycle 1 CompleteAddressed:
Intentionally ignored: <!-- claude-review-cycle --> |
…tive # Conflicts: # docs/primitives.md
Implements the
cronjobKubernetes resource primitive following the pattern established by the existingConfigMapandDeploymentprimitives.Summary
cronjobprimitive package underpkg/primitives/cronjob/Checklist